sBTC Alpha Testnet Contract #3499

xoloki · 2023-01-18T00:04:16Z

Description

Add a simple contract to allow administering the coordinator and signer keys, and also allowing mint/burn/transfer of sBTC tokens.

Applicable issues

fixes [sBTC] Add minimal contract to allow token and key administration #3488

Additional info (benefits, drawbacks, caveats)

This contract is much more limited than the final one will be. It does not allow separate admins from the contract owner, and does not allow signers to rotate their keys. It also has neither a way to verify schnorr sigs, nor contract code to allow automatic peg handing once a threshold is reached.

Checklist

Test coverage for new or modified code paths
Changelog is updated
Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
New clarity functions have corresponding PR in clarity-benchmarking repo
New integration test(s) added to bitcoin-tests.yml

codecov · 2023-01-18T00:06:45Z

Codecov Report

Merging #3499 (d419542) into next (e27cdf6) will decrease coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             next    #3499      +/-   ##
==========================================
- Coverage   31.15%   31.06%   -0.10%     
==========================================
  Files         305      305              
  Lines      276796   276796              
==========================================
- Hits        86233    85982     -251     
- Misses     190563   190814     +251

Impacted Files	Coverage Δ
src/net/codec.rs	`40.49% <0.00%> (-1.56%)`	⬇️
src/chainstate/coordinator/mod.rs	`80.96% <0.00%> (-1.56%)`	⬇️
src/monitoring/mod.rs	`60.26% <0.00%> (-1.31%)`	⬇️
src/net/p2p.rs	`51.99% <0.00%> (-1.16%)`	⬇️
src/net/relay.rs	`22.39% <0.00%> (-1.04%)`	⬇️
src/net/chat.rs	`30.16% <0.00%> (-0.73%)`	⬇️
.../chainstate/burn/operations/leader_block_commit.rs	`15.08% <0.00%> (-0.72%)`	⬇️
src/util_lib/db.rs	`72.08% <0.00%> (-0.66%)`	⬇️
clarity/src/vm/database/key_value_wrapper.rs	`95.56% <0.00%> (-0.64%)`	⬇️
src/chainstate/stacks/index/cache.rs	`13.54% <0.00%> (-0.51%)`	⬇️
... and 27 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

contrib/sbtc/key-admin/contracts/key-admin.clar

contrib/sbtc/token-key-admin/contracts/sbtc-token-key-admin.clar

jcnelson · 2023-01-19T20:22:41Z

contrib/sbtc/token-key-admin/contracts/sbtc-token-key-admin.clar

+
+;; data vars
+;;
+(define-data-var coordinator (optional principal) none)


Would it make sense to just initialize coordinator to be the intended principal from the get-go, instead of requiring the coordinator to call set-coordinator-key?

This would also remove the need to represent coordinator as an optional.

We were assuming that the principal deploying the contract would be an admin, but not necessarily the coordinator. And since we are going to need separate FROST public keys in addition to principals, I'm not sure there's a good way to set that on contract init.

Can we initialize coordinator to the contract admin/creator? The admin can later change it and this way the variable doesn't need to be optional.

Can we initialize coordinator to the contract admin/creator? The admin can later change it and this way the variable doesn't need to be optional.

No, because we need a FROST public key in addition to a principal.

contrib/sbtc/token-key-admin/contracts/sbtc-token-key-admin.clar

xoloki · 2023-01-20T16:48:14Z

@igorsyl @jcnelson are we happy with the current implementation for alpha? If not, what other changes would you like to see?

contrib/sbtc/token-key-admin/contracts/sbtc-token-key-admin.clar

igorsyl · 2023-01-20T18:13:35Z

We need to add a data member to store frost params N and T and ability for owner to reset these params. The signer set function should check the id is in [0,N].

Also, it may be useful to have functions to reset the contract-owner.

Also, we need to a member variable to store the threshold wallet bitcoin address gated by the coordinator.

igorsyl · 2023-01-20T18:19:16Z

contrib/sbtc/token-key-admin/contracts/sbtc-token-key-admin.clar

+
+;; private functions
+;;
+(define-private (is-valid-caller)


Could we rename to this function assert-tx-sender-is-owner! and add assert-tx-sender-is-coordinator!?

I named them is-contract-owner and is-coordinator since we might want to call them when we aren't doing an assert.

Makes sense. Could we have assert-is-contract-owner call is-contract-owner?

I'm getting a weird clarinet check error when I try to add these functions.

(define-private (assert-is-contract-owner) (asserts! (is-eq contract-owner tx-sender) (err err-invalid-caller)) )

detected two execution paths, returning two different expression types (got '(response UnknownType uint)' and 'bool')

No, same error @igorsyl

I think I understand; I'm guessing that asserts! in the true case returns true from the statement, and in the error case return err from the function. Since the compiler can't know which branch will be followed there is a conflict.

When using asserts! in the body of a function, though, the statement return from asserts! is ignored, but the function will return the error.

Yeah that was it

So the following does work:

(define-private (assert-is-coordinator) (begin (asserts! (is-coordinator) (err err-invalid-caller)) (ok true) ) )

But this won't work if we replace the existing asserts! calls, since we will now be returning a result in either case, and not immediately returning from the function in the error case. I'll still need to wrap in the same asserts! to get the existing behavior.

Maybe if I made assert-is-contract-owner a macro? Is this what the ! token indicates? What about the ? token?

That doesn't work either:
(define-public (set-contract-owner (owner principal)) (begin (asserts-is-contract-owner!) (ok (var-set contract-owner owner)) ) )
(define-private (asserts-is-contract-owner!) (begin (asserts! (is-contract-owner) (err err-invalid-caller)) (ok true) ) )
Still gives a clarity error:

intermediary responses in consecutive statements must be checked

contrib/sbtc/token-key-admin/contracts/sbtc-token-key-admin.clar

xoloki · 2023-01-20T21:50:01Z

We need to add a data member to store frost params N and T and ability for owner to reset these params. The signer set function should check the id is in [0,N].

Also, it may be useful to have functions to reset the contract-owner.

Also, we need to a member variable to store the threshold wallet bitcoin address gated by the coordinator.

Added num-keys num-parties and threshold with accessors and tests. I'll add the rest next.

xoloki · 2023-01-20T21:53:04Z

BTW clarinet check is spamming warning: use of potentially unchecked data on all my setters. What's the idiomatic way of checking data? @igorsyl @jcnelson

xoloki · 2023-01-23T20:29:41Z

We need to add a data member to store frost params N and T and ability for owner to reset these params. The signer set function should check the id is in [0,N].
Also, it may be useful to have functions to reset the contract-owner.
Also, we need to a member variable to store the threshold wallet bitcoin address gated by the coordinator.

Added num-keys num-parties and threshold with accessors and tests. I'll add the rest next.

signer set/delete now checks for valid id

xoloki · 2023-01-23T20:35:23Z

We need to add a data member to store frost params N and T and ability for owner to reset these params. The signer set function should check the id is in [0,N].
Also, it may be useful to have functions to reset the contract-owner.
Also, we need to a member variable to store the threshold wallet bitcoin address gated by the coordinator.

Added num-keys num-parties and threshold with accessors and tests. I'll add the rest next.

contract-owner is now a var and can be set

xoloki · 2023-01-23T20:38:05Z

We need to add a data member to store frost params N and T and ability for owner to reset these params. The signer set function should check the id is in [0,N].

Also, it may be useful to have functions to reset the contract-owner.

Also, we need to a member variable to store the threshold wallet bitcoin address gated by the coordinator.

What's the idiomatic way of storing a bitcoin address @igorsyl ?

igorsyl · 2023-01-23T22:47:14Z

@lgalabru ?

BTW clarinet check is spamming warning: use of potentially unchecked data on all my setters. What's the idiomatic way of checking data? @igorsyl @jcnelson

… add test that tries to transfer once trading is halted (currently fails in what appears to be infinite recursion

netrome · 2023-01-30T14:55:48Z

contrib/sbtc/token-key-admin/contracts/sbtc-token-key-admin.clar

+(define-public (mint! (amount uint) (memo (string-ascii 72)))
+    (begin
+        (asserts! (is-coordinator) (err err-invalid-caller))
+        (ft-mint? sbtc amount tx-sender)
+    )
+)
+
+(define-public (burn! (amount uint) (memo (string-ascii 72)))
+    (begin
+        (asserts! (is-coordinator) (err err-invalid-caller))
+        (ft-burn? sbtc amount tx-sender)
+    )
+)


These methods should take an additional principal as an argument, since the coordinator will mint & burn tokens for arbitrary stacks principals.

I think memo is meant to encode the bitcoin ops request txid.

Done @netrome

See below @igorsyl , I used peg-in/out-txid for mint/burn

igorsyl · 2023-01-30T16:43:32Z

contrib/sbtc/token-key-admin/contracts/sbtc-token-key-admin.clar

+    )
+)
+
+(define-public (burn! (amount uint) (memo (string-ascii 72)))


can we instead of calling it memo call it peg_out_txid

Done, used peg-in/out-txid for mint/burn

xoloki · 2023-01-30T21:50:03Z

Ok @igorsyl @netrome the contract and tests are all passing, I think I've handled all review comments. Please let me know if you need anything else.

igorsyl · 2023-01-30T22:27:23Z

Ok @igorsyl @netrome the contract and tests are all passing, I think I've handled all review comments. Please let me know if you need anything else.

I'll deploy this. We can keep the PR open. Thanks @xoloki

xoloki · 2023-01-30T22:28:32Z

Ok @igorsyl @netrome the contract and tests are all passing, I think I've handled all review comments. Please let me know if you need anything else.

I'll deploy this. We can keep the PR open. Thanks @xoloki

Thanks @igorsyl

igorsyl · 2023-03-04T17:22:35Z

The sbtc contract is being developed here. We will move it to this repository once it's stable.

blockstack-devops · 2024-11-13T00:21:32Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

xoloki requested review from igorsyl and jcnelson January 18, 2023 00:36

xoloki marked this pull request as ready for review January 18, 2023 00:46

igorsyl reviewed Jan 18, 2023

View reviewed changes

contrib/sbtc/key-admin/contracts/key-admin.clar Outdated Show resolved Hide resolved

igorsyl reviewed Jan 18, 2023

View reviewed changes

contrib/sbtc/key-admin/contracts/key-admin.clar Outdated Show resolved Hide resolved

igorsyl reviewed Jan 18, 2023

View reviewed changes

contrib/sbtc/key-admin/contracts/key-admin.clar Outdated Show resolved Hide resolved

jcnelson reviewed Jan 19, 2023

View reviewed changes